Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Always show Create App button in empty state (even on error) - Add System Settings link to empty state pages - Add top-level /system/* and /create-app routes for access without app context - Modify AppSidebar to show system navigation when no apps configured - Update SystemHubPage and AppManagementPage to handle missing appName Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Validates: create-app button always visible, system settings link present, system routes accessible when no active app exists. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
- Simplify tooltip and label rendering for fallback nav items - Reduce test timeouts from 10000ms to 5000ms - Update ROADMAP.md with P1.12.1 empty state improvements Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 19:40:26.042 @object-ui/plugin-view:build: �[96msrc/ObjectView.tsx�[0m:�[93m987�[0m:�[93m32�[0m - �[91merror�[0m�[90m TS6133: �[0m'record' is declared but its value is never read. |
- Prefix unused `record` param with underscore in ObjectView.tsx - Remove unused `fireEvent` import from EmptyStateSystemRoutes.test.tsx Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Fixed both TS6133 errors in commit
|
There was a problem hiding this comment.
Pull request overview
This PR improves the console’s “no apps configured” experience by adding prominent CTAs for creating an app and accessing System Settings, and by promoting system administration pages to be reachable via top-level routes (without needing an app context).
Changes:
- Adds top-level
/system/*and/create-approutes and updates empty states to link to them. - Updates system pages to build links correctly when
appNameis absent (top-level access). - Adds a new test suite for no-app empty state and in-app-context system/create-app routing, plus updates ROADMAP and fixes a TS unused param.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugin-view/src/ObjectView.tsx | Renames an unused parameter to avoid TS unused-var errors. |
| apps/console/src/pages/system/SystemHubPage.tsx | Supports top-level system routes by handling missing appName. |
| apps/console/src/pages/system/AppManagementPage.tsx | Supports top-level system routes by handling missing appName. |
| apps/console/src/components/AppSidebar.tsx | Adds fallback “System” navigation UI when no active app exists. |
| apps/console/src/tests/EmptyStateSystemRoutes.test.tsx | Adds tests for empty state CTAs and system/create-app routes under /apps/:appName/* when no apps exist. |
| apps/console/src/App.tsx | Updates empty states, adds top-level system/create-app routes, and adds a SystemRoutes route group. |
| ROADMAP.md | Documents completion of empty-state + system-route accessibility work. |
|
|
||
| if (!activeApp && !isCreateAppRoute) return ( | ||
| // Check if we're on a system route (accessible without an active app) | ||
| const isSystemRoute = location.pathname.includes('/system'); |
There was a problem hiding this comment.
isSystemRoute uses location.pathname.includes('/system'), which can match unrelated paths (e.g. /apps/system/... where appName is system, or /apps/_new/systematic). Since this logic gates whether we bypass the empty-state screen when !activeApp, it should match a full path segment (or use useParams().appName and check pathname.startsWith(/apps/${appName}/system)) rather than a substring match.
| const isSystemRoute = location.pathname.includes('/system'); | |
| const isSystemRoute = | |
| /^\/system(\/|$)/.test(location.pathname) || | |
| /^\/apps\/[^/]+\/system(\/|$)/.test(location.pathname); |
| // When on create-app without an active app, render a minimal layout with just the wizard | ||
| if (!activeApp && isCreateAppRoute) { | ||
| if (!activeApp && (isCreateAppRoute || isSystemRoute)) { | ||
| return ( | ||
| <Suspense fallback={<LoadingScreen />}> | ||
| <Routes> | ||
| <Route path="create-app" element={<CreateAppPage />} /> | ||
| <Route path="system" element={<SystemHubPage />} /> | ||
| <Route path="system/apps" element={<AppManagementPage />} /> | ||
| <Route path="system/users" element={<UserManagementPage />} /> | ||
| <Route path="system/organizations" element={<OrgManagementPage />} /> | ||
| <Route path="system/roles" element={<RoleManagementPage />} /> | ||
| <Route path="system/permissions" element={<PermissionManagementPage />} /> | ||
| <Route path="system/audit-log" element={<AuditLogPage />} /> | ||
| <Route path="system/profile" element={<ProfilePage />} /> | ||
| </Routes> |
There was a problem hiding this comment.
When !activeApp and the route is create-app or a system route, this returns a minimal <Routes> block without ConsoleLayout/AppSidebar. That means the newly added no-app fallback navigation in AppSidebar will never be rendered in the no-app scenarios this PR targets. Consider rendering these routes inside the normal shell (or a dedicated system shell that includes the sidebar) so users still have consistent navigation.
| {/* Top-level system routes — accessible without any app */} | ||
| <Route path="/system/*" element={ | ||
| <AuthGuard fallback={<Navigate to="/login" />} loadingFallback={<LoadingScreen />}> | ||
| <ConnectedShell> | ||
| <SystemRoutes /> | ||
| </ConnectedShell> | ||
| </AuthGuard> | ||
| } /> | ||
| {/* Top-level create-app — accessible without any app */} | ||
| <Route path="/create-app" element={ | ||
| <AuthGuard fallback={<Navigate to="/login" />} loadingFallback={<LoadingScreen />}> | ||
| <ConnectedShell> | ||
| <Suspense fallback={<LoadingScreen />}> | ||
| <CreateAppPage /> | ||
| </Suspense> | ||
| </ConnectedShell> | ||
| </AuthGuard> | ||
| } /> |
There was a problem hiding this comment.
New top-level /system/* and /create-app routes are introduced here, but the added test file only mounts AppContent under /apps/:appName/*, so it doesn’t exercise these top-level routes. Add a test that mounts the route config used by App() (or refactor route definitions for testability) to verify /system and /create-app work without an app context and remain guarded by auth as intended.
| <SidebarMenuButton asChild tooltip={item.label as string}> | ||
| <Link to={(item as any).url || '/system'}> | ||
| <NavIcon className="h-4 w-4" /> | ||
| <span>{item.label as string}</span> |
There was a problem hiding this comment.
Avoid (item as any).url / item.label as string casts here. NavigationItem already has an optional url?: string, and label may be an i18n object. Use item.url ?? '/system' and resolveI18nLabel(item.label, t) (or define systemFallbackNavigation with a narrower type) so the fallback nav stays type-safe and won’t silently break if labels become localized later.
| <SidebarMenuButton asChild tooltip={item.label as string}> | |
| <Link to={(item as any).url || '/system'}> | |
| <NavIcon className="h-4 w-4" /> | |
| <span>{item.label as string}</span> | |
| <SidebarMenuButton asChild tooltip={resolveI18nLabel(item.label, t)}> | |
| <Link to={item.url ?? '/system'}> | |
| <NavIcon className="h-4 w-4" /> | |
| <span>{resolveI18nLabel(item.label, t)}</span> |
| * and the availability of system routes and create-app entry points. | ||
| * | ||
| * Requirements: | ||
| * - "Create App" button always visible in empty state (even on error) | ||
| * - "System Settings" link always visible in empty state | ||
| * - System routes accessible without app context | ||
| * - Login/Register/Forgot password always accessible |
There was a problem hiding this comment.
The header comment lists Login/Register/Forgot Password accessibility as a requirement, but this test file doesn’t include assertions for those routes (and it also doesn’t cover the new top-level /system/* routes added in App()). Either add the missing tests or trim the stated requirements so the comment accurately reflects what’s being validated.
| * and the availability of system routes and create-app entry points. | |
| * | |
| * Requirements: | |
| * - "Create App" button always visible in empty state (even on error) | |
| * - "System Settings" link always visible in empty state | |
| * - System routes accessible without app context | |
| * - Login/Register/Forgot password always accessible | |
| * and the availability of core system navigation and create-app entry points. | |
| * | |
| * This suite focuses on the empty state UI and key system navigation links | |
| * that should remain available even when no applications are configured. |
| return ( | ||
| <Suspense fallback={<LoadingScreen />}> | ||
| <Routes> | ||
| <Route path="/" element={<SystemHubPage />} /> |
There was a problem hiding this comment.
In SystemRoutes(), the child route for the hub uses path="/", which is treated as an absolute path in React Router. When this component is mounted under the parent route /system/*, /system may not match this child route and can render a blank page. Use an index route (or path="") for the default /system page instead.
| <Route path="/" element={<SystemHubPage />} /> | |
| <Route index element={<SystemHubPage />} /> |
RootRedirectin App.tsx — Always show "Create App" button (even on error) + add "System Settings" linkAppContentempty state in App.tsx — Show "Create App" + "System Settings" buttons; allow system routes when no active app/system/*and/create-approutes inApp()— So system pages are accessible without any app contextAppSidebar.tsx— Handle no-app fallback with system navigation items (System Settings, Create App, Users, Orgs, Roles)SystemHubPage.tsx— Handle missingappNameparam for top-level route accessAppManagementPage.tsx— Handle missingappNameparam for top-level route accessrecordin ObjectView.tsx, unusedfireEventimport in testOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.